Skip to content

Warn FS3888 when consumer-visible attributes are missing from .fsi#19880

Merged
T-Gro merged 70 commits into
mainfrom
fix/issue-19560
Jun 12, 2026
Merged

Warn FS3888 when consumer-visible attributes are missing from .fsi#19880
T-Gro merged 70 commits into
mainfrom
fix/issue-19560

Conversation

@T-Gro

@T-Gro T-Gro commented Jun 2, 2026

Copy link
Copy Markdown
Member

Fixes #19560

Tooling and consumer code typecheck against the .fsi, not the .fs.
Compiler-semantic attributes (e.g. [<NoDynamicInvocation>], [<AutoOpen>],
[<IsByRefLike>]) that live only on the implementation are silently dropped,
so the contract the consumer typechecks against diverges from the one emitted
at runtime. FS3888 now flags every paired .fs/.fsi symbol whose impl carries
a typecheck-affecting attribute the signature lacks; under the
ErrorOnMissingSignatureAttribute preview language feature it is an error.

Two VS lightbulbs are offered on the diagnostic: add the attribute to the
.fsi, or remove it from the .fs.

Copilot and others added 4 commits June 2, 2026 14:46
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>


Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required

You can open this PR in browser to add release notes: open in github.dev


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/11.0.100.md
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md
LanguageFeatures.fsi docs/release-notes/.Language/preview.md
vsintegration/src docs/release-notes/.VisualStudio/18.vNext.md

@auduchinok auduchinok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copilot and others added 2 commits June 2, 2026 18:30
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 2, 2026
@T-Gro T-Gro requested a review from abonie June 3, 2026 10:14
Comment thread src/Compiler/FSComp.txt Outdated

@abonie abonie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay approving as is, but worth considering if we can add a code fix for this error and/or make it a warning first (then upgrade to error later on)

…(1) happy path (issue #19560)

Change the signature conformance enforcement introduced in PR #19880 from
a hard error to a warning prefixed with 'This will become an error in
future versions of F#' so existing libraries are not broken in-place.

Refactor the enforcement so the happy path is a single O(1) bitmask
check: combine every enforced flag with bitwise OR into one
WellKnownValAttributes / WellKnownEntityAttributes mask, and only walk
the per-attribute list (also O(1) per row) when the mask matches.

Extend the enforced set beyond NoDynamicInvocation to cover every
attribute whose presence on the signature is observed by the typecheck
or compilation of separate consumer code. Per-Val: NoDynamicInvocation,
RequiresExplicitTypeArguments, Conditional, NoEagerConstraintApplication,
GeneralizableValue, WarnOnWithoutNullArgument, CLIEvent. Per-Entity (now
also enforced on nested modules, not just types): RequireQualifiedAccess,
AutoOpen, NoComparison, NoEquality, AbstractClass, Sealed (three-state),
CLIMutable, AllowNullLiteral (three-state), DefaultAugmentation
(three-state), Obsolete, CompilerMessage, Experimental, Unverifiable,
EditorBrowsable, AttributeUsage, and
CompilationRepresentation(UseNullAsTrueValue).

Adding a new enforced attribute is now a one-line edit to a single
in-code list; the all-up mask and per-attribute diagnostic are derived
automatically.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@auduchinok

Copy link
Copy Markdown
Member

Let's make it an error guarded with a language version?

T-Gro and others added 12 commits June 5, 2026 11:10
…19560)

Replace the multi-line attribute-list bodies in SignatureConformance.fs
with a tiny DSL that declares one rule per line:

- EnforcedPhase (TypeCheck | CodeGen | TypeCheckAndCodeGen | Indirect)
  documents which consumer compile-stage actually reads the attribute,
  so reviewers can argue per-row.
- valOne / valPair / entOne / entPair are the row constructors. Pair
  handles three-state bools (_True ||| _False).
- V / E are short local type aliases for WellKnownValAttributes and
  WellKnownEntityAttributes so each row fits on one line.

The list/mask shape and runtime semantics are unchanged: the all-up
mask is still computed by folding the rows, the happy path is still a
single O(1) HasWellKnownAttribute(mask) check, and the slow path still
iterates the (small) row list emitting one warning per missing
attribute. All existing tests pass (26/26 in Conformance.Signatures*).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…y (issue #19560)

Drop the EnforcedPhase tag, the per-row trailing comments, and the
val/ent/pair helpers. The policy is now a plain list of (flag, name)
tuples — one row per line — with V/E type aliases for terseness and a
single fold to compute the O(1) early-exit mask. Behaviour unchanged
(26/26 Conformance.Signatures tests pass).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ame (issue #19560)

Each row is now a single enum value. The diagnostic name is derived
from the enum case (`AutoOpenAttribute` -> `AutoOpen`,
`SealedAttribute_True ||| _False` -> `Sealed` via lowest-set-bit).
Dropped the CompilationRepresentation_PermitNull row because (a) the
enum case does not match the user-written attribute name, and (b) it
is IL-codegen-only.

Behaviour unchanged for the 15 covered cases (all enforcement tests
still pass).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…19560)

Previously: 1 mask check on impl (fast), then a per-flag loop on EACH
enforced row even when sig already carried the bits — slow for libs
that did the right thing.

Now: mask check on impl; on hit, mask check on sig (forces caching),
then a single bitwise diff missing = impl & mask & ~(sig & mask). Per-
attribute loop only runs when missing != 0 (true mismatch).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19560)

Add a small `Flags` module in WellKnownAttribs.{fs,fsi} with set-op
primitives over any uint64-backed enum: isEmpty, union, intersect,
except, intersects, isSubsetOf. All inline, all reducing to single
uint64 ALU ops after JIT.

Use them in SignatureConformance.fs so the enforcement reads as set
operations:

  let implOnEnforced = impl.Flags |> Flags.intersect enforcedMask
  let sigOnEnforced  = sig.Flags  |> Flags.intersect enforcedMask
  if not (implOnEnforced |> Flags.isSubsetOf sigOnEnforced) then
      let missing = implOnEnforced |> Flags.except sigOnEnforced
      for flag in policy do
          if flag |> Flags.intersects missing then warn flag

Behaviour unchanged; 26/26 Conformance.Signatures tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ntInSig (issue #19560)

The new names encode the asymmetry of the check directly: the impl
side's enforced bits are what's required from the sig, and we compare
against what is actually present in the sig.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560)

Wrap the `HasWellKnownAttribute ... |> ignore` cache-populating side-
effect into named helpers `enforcedFlagsOnVal` / `enforcedFlagsOnEntity`
that return the flags intersected with the enforcement mask. Call sites
now read as plain set operations with no `|> ignore` leak.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560)

The Val and Entity enforcement blocks were structurally identical bar
flag type, per-subject flag fetcher, policy list, and display-name
accessor. Lift them into a single inline generic helper
`checkEnforcedAttribs` parameterised on all four. Val and Entity
specialisations are one-line partial applications.

Inline + 'F : enum<uint64> inference keeps the bit math zero-cost.
26/26 Conformance.Signatures tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ormance module + range/placement tests (issue #19560)

- Add `LanguageFeature.ErrorOnMissingSignatureAttribute` (preview). When
  the language version supports it, FS3888 escalates from warning to
  error; otherwise it stays a suppressible warning.
- Extract the policy and matching logic into nested
  `module private AttributeConformance =` inside SignatureConformance.
  The Checker now calls `AttributeConformance.checkVal` /
  `checkEntity` and is no longer entangled with the bit math.
- Point the diagnostic squiggle at the offending attribute in the .fs
  (via `Attrib.Range`) instead of the value/type identifier, so the
  IDE highlights exactly the attribute the user must mirror.
- Add tests for: module-level attribute, diagnostic placement on the
  attribute (range start/end on the attribute's line, not the val/type
  identifier line), and language-feature-driven escalation to error.
  19/19 enforcement tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#19560)

Adds `AddMissingAttributeToSignatureCodeFixProvider` for FS3888. The
diagnostic is reported on the attribute in the .fs; the fix inserts
the same attribute text into the .fsi above the matching declaration,
preserving the sig line's indentation.

Cross-document mechanics (no prior art in this repo - `RenameParam-
ToMatchSignature` is sig-aware but rewrites the local .fs only):
- Locate the symbol the attribute attaches to via lexer lookup at the
  next non-whitespace position after the diagnostic span.
- Resolve the sig location via `FSharpSymbol.SignatureLocation`.
- Find the .fsi document in the solution by file path.
- Compute insertion: start of the sig declaration's line + leading
  whitespace from that line for the new attribute line.
- Apply via `CodeAction.Create` with a `ChangedSolution` producer
  (Roslyn's `Document.WithText` -> `Project.Solution`).

Modeled after `MissingReference.fs` (the only existing fix that
overrides `RegisterCodeFixesAsync` directly and uses
`cancellableTask` + `CancellableTask.startAsTask`).

VS integration is Windows-only; cannot be built on macOS due to
missing .NETFramework 4.7.2 reference assemblies. Windows CI will
verify the build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ssue #19560)

The diagnostic range (Attrib.Range / SynAttribute.Range) covers ONE
attribute body WITHOUT the surrounding `[< >]` brackets and without
sibling attributes in a `[<A; B>]` list. So the verbatim copy gives
`NoDynamicInvocation(false)` - the code-fix now wraps with `[< >]`
before inserting into the .fsi.

The skip-forward loop after the diagnostic span now also skips `>`,
`]`, `;` to land on the val/type/member ident correctly when the
diagnostic targets one attribute inside a `[<A; B>]` list.

Tests (vsintegration/tests/.../CodeFixes/AddMissingAttributeToSignatureTests.fs)
cover: module-level (AutoOpen on nested module), type-level
(RequireQualifiedAccess on union), function-level (NoDynamicInvocation
on val), attribute-with-argument (AllowNullLiteral(false) - exercises
verbatim arg copying).

The existing CodeFixTestFramework assumes IFSharpCodeFixProvider with
single-doc TextChange list, which doesn't fit a cross-document fix.
Tests use a small inline harness that invokes RegisterCodeFixesAsync
directly, captures the CodeAction, applies it via ApplyChangesOperation
and diffs the .fsi document in the resulting Solution.

VS integration cannot be built on macOS (missing .NETFramework 4.7.2
reference assemblies); Windows CI verifies the build + tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ribute tests (issue #19560)

Addresses adversarial review findings:

1. Symbol-lookup was unreliable (skip-forward landed on `let`/`type`
   keywords or sibling attributes in [<A; B>] / [<A>][<B>] cases).
   Replace with check-results-based enumeration: iterate
   `GetAllUsesOfAllSymbolsInFile`, filter for `IsFromDefinition &&
   Symbol.SignatureLocation.IsSome`, pick the first whose range starts
   on/after the diagnostic attribute's end position. Locates the val/
   type/module regardless of attribute syntax.

2. EOL: use the .fsi file's existing line break (from the target line's
   `EndIncludingLineBreak` span) instead of `Environment.NewLine`,
   so LF-only files stay LF and CRLF files stay CRLF.

3. Path comparison: case-insensitive only on Windows; case-sensitive on
   POSIX so Roslyn matches behaviour of the filesystem.

4. Equivalence key now includes the sig location to prevent collision
   when the same attribute text is missing on multiple unrelated decls.

5. Tests extended with multi-attribute coverage:
   - Two stacked [<A>]\n[<B>] both missing -> two independent fixes.
   - Two on one line [<A; B>] both missing -> two independent fixes.
     Each insertion is its own [<X>] (per-attribute wrap by design).
   - Mixed enforced + non-enforced on same line - only enforced inserted.
   - .fsi already carries a non-enforced attribute -> new one added,
     existing one preserved.
   - Test harness now exposes `tryFixSigAt diagIndex` so multi-diag
     scenarios can be exercised.

Compiler-side: 30/30 Conformance.Signatures tests still pass.

VS integration build is Windows-only (.NETFramework 4.7.2 reference
assemblies missing on macOS); Windows CI verifies the code-fix + tests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro T-Gro changed the title Enforce compiler-semantic attributes in signature conformance (issue #19560) Warn FS3888 when consumer-visible attributes are missing from .fsi Jun 9, 2026
T-Gro and others added 15 commits June 9, 2026 15:13
Drop bloat across release notes: the inline enforced-attribute list, the
'internal vs public' aside, and the runtime-skip clause are not consumer-
facing release-note material - they belong in the diff or the issue.

- FSharp.Compiler.Service: 25-line paragraph -> 3 sentences.
- Language preview: trim one-line entry; mention what FS3888 is so the
  bullet stands on its own.
- VisualStudio: collapse two 4-line code-fix bullets into one self-standing
  bullet ("copy the attribute into the .fsi, or remove it from the .fs").
- FSharp.Core: drop the entry entirely - it was an internal mirror change
  with no consumer-visible behavior delta.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Captures recurring review feedback distilled from 3 weeks of sessions:
- Good names beat comments. Don't restate the code, don't narrate the
  algorithm, don't justify decisions inline, no war-story comments.
- Compact idiomatic F#: pattern matching, active patterns, extracted
  helpers, low cyclomatic complexity, new file > bloating a huge file.
- Tests: parametrized > copy-pasted, module-level shared constants,
  helpers like parseAndCheck.
- PR scope: not paid by LOC. Cleanup commits separate from feature
  commits. No 'phase tag' / 'transitional measure' breadcrumbs.

Auto-applied via applyTo frontmatter to src/Compiler, vsintegration/src,
and the F# test suites. Verified the YAML parses identically to the
existing ExpertReview / ComponentTests instruction files.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#19560)

main brought in new bare 'equals m range0' call sites in
CodeGen/IlxGen.fs (DebugPoint handling for both Expr and LinearExpr)
and Checking/NameResolution.fs (TypeVar range comparison) after this
branch added [<AutoOpen>] to TaggedText.fsi. With TaggedText auto-opened,
bare 'equals' now resolves to the TaggedText value, not Range.equals.
Qualify the three sites.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rom-main attribs (issue #19560)

After merge from main, several new types/attributes surfaced FS3888 across
the F# self-build. Adjustments:

SignatureConformance.fs:
  * Skip the check when the sig declares the entity with hidden representation
    (e.g. `type internal T` opaque in .fsi vs DU in .fs). Attribute-mirroring
    is syntactically impossible on opaque sig declarations.
  * Drop AutoOpen from the enforced set: a legitimately asymmetric idiom on
    internal modules (auto-open within the project, opaque for
    InternalsVisibleTo consumers and for the .fsi-visible surface).
  * Drop StructuralEquality / StructuralComparison from the enforced set: they
    are documentary, matching the F# default for DU/record/struct types; their
    presence on .fs and absence on .fsi has no observable consumer effect.

Mirror impl attribs into the matching .fsi where the sig was simply missing
them: NoEquality/NoComparison on DisplayEnv (TypedTreeOps.FreeVars.fsi) and
ExprRewritingEnv (TypedTreeOps.Transforms.fsi); RequireQualifiedAccess on
LexerEndlineContinuation (ParseHelpers.fsi).

il.fs: remove the dead [<RequireQualifiedAccess>] from ILSecurityDecl
(the DU case is used unqualified in ilwrite.fs).

Tests:
  * AutoOpen tests now assert no FS3888 (asymmetric is intended).
  * StructuralEquality test now asserts no FS3888 (documentary attribute).

Self-build clean after `git clean -xfd artifacts` (stale binaries from prior
attempts had the old AutoOpen attribute baked in - clean rebuild required).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dAccess>] (issue #19560)

The two failing tests in WindowsCompressedMetadata vs_release used [<AutoOpen>]
on a module to trigger FS3888 and verify the cross-document fix. AutoOpen was
intentionally removed from the enforced attribute set (legitimately asymmetric
on internal modules), so the diagnostic no longer fires for these inputs and
the code-fix has nothing to do.

Swap to [<RequireQualifiedAccess>] which IS enforced - the codefix-on-module
coverage is preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…odules) (issue #19560)

The previous hidden-repr skip used `not sigEntity.IsHiddenReprTycon` to avoid
firing on opaque type declarations (`type internal T` in .fsi vs DU in .fs).
But IsHiddenReprTycon also returns true for modules, which incorrectly silenced
RequireQualifiedAccess enforcement on modules - that broke the
`Module-level: ...` VS code-fix tests because the diagnostic never fired so
the fix had nothing to do.

Refine: check modules unconditionally, only skip non-module hidden-repr tycons.

Adds a conformance test that verifies RequireQualifiedAccess on a nested
module in impl but not sig actually fires FS3888.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ules (issue #19560)

After the previous commit started enforcing RQA on modules (regression from
the over-broad IsHiddenReprTycon skip), the F# self-build surfaced four
internal modules with RQA on .fs but not .fsi:

- DiagnosticsLogger.fsi: module OperationResult
- PrettyNaming.fsi: module internal CustomOperations
- WarnScopes.fsi: module internal WarnScopes
- FileSystem.fsi: module internal FileSystemUtils

All call sites were already qualified, so adding RQA to the .fsi is a no-op
behaviour-wise. Self-build clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… qualify call sites (issue #19560)

These two DU types had RQA in the .fs (intentional - the cases Encoded/Decoded
and the single ILSecurityDecl case are generic enough that consumers should
qualify). I had removed RQA earlier to silence FS3888 without migrating the
unqualified call sites - that defeated the whole point of the attribute.

Restored RQA on both types in .fs and .fsi; migrated:
- src/Compiler/CodeGen/EraseUnions.fs       Encoded(...)        -> ILAttribute.Encoded(...)
- src/Compiler/AbstractIL/ilprint.fs        ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...)
- src/Compiler/AbstractIL/ilread.fs         ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...)
- src/Compiler/AbstractIL/ilwrite.fs        ILSecurityDecl(...) -> ILSecurityDecl.ILSecurityDecl(...)

XmlDoc is a class, not a DU; RQA on classes is a no-op so the earlier removal stays.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(issue #19560)

I had removed it from sformat.fs IEnvironment earlier to silence FS3888,
which is precisely backwards - the attribute was deliberately there. Restore
on both .fs and .fsi. The earlier attempt at this same fix appeared to break
the test framework, but that turned out to be stale artifacts from a prior
build with [<AutoOpen>] on Layout. A clean rebuild verifies the fix is clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… set

Replace the vague 'legitimately asymmetric idiom' justification with the
empirically-verified reasons:

AutoOpen on .fs alone is a no-op for F# consumers - the .fsi governs the
FSharpSignatureData consumers read, the IL attribute is ignored. (Proven
with a 4-variant fs-only/fsi-only/both/neither test against an external
consumer.)

StructuralEquality/StructuralComparison are documentary at the consumer
boundary - the F# default for DU/record/struct already generates the same
Equals/GetHashCode/CompareTo and IStructural* interfaces. The attribute IS
load-bearing as a compile-time assertion (FS1176/FS1177 when a field
doesn't satisfy equality/comparison) but that fires at the .fs's own
compile, not at consumers.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Break long match-case line in EraseUnions.fs to stay within 140-char limit
- Add release note entry for FSharp.Core .fsi attribute mirroring

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…1458262, 1457817)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@T-Gro

T-Gro commented Jun 12, 2026

Copy link
Copy Markdown
Member Author

@abonie : 2 codefixes added

@auduchinok : In the end I left AutoOpen out of it - this codebases also uses the "trick" of autopen within the .fs it exists in, but not to the outside world.

Cut multi-line restate-the-code prose and 'will become an error
later' / 'same rationale as' breadcrumbs. Keep only one-line
'why' notes where the why is non-evident.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment thread .github/instructions/NoBloat.instructions.md Outdated
@github-project-automation github-project-automation Bot moved this from New to In Progress in F# Compiler and Tooling Jun 12, 2026
@T-Gro T-Gro enabled auto-merge (squash) June 12, 2026 09:37
@T-Gro T-Gro merged commit 14c4ced into main Jun 12, 2026
50 checks passed
@T-Gro T-Gro deleted the fix/issue-19560 branch June 12, 2026 10:21
T-Gro pushed a commit that referenced this pull request Jun 12, 2026
After merging main, FS3888 was taken by PR #19880 (implAttributeMissingFromSignature).
The namespace/type collision diagnostic was renumbered to FS3889 in FSComp.txt,
but the test and release notes still referenced the old number.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Attributes from implementation file are not seen by the compiler

3 participants